Skip to content

Conversation

@dom96
Copy link
Contributor

@dom96 dom96 commented Oct 24, 2025

Test is in EW because wd-test doesn't make it easy to assert on script startup failures afaik.

Test Plan

$ bazel test //src/edgeworker/tests/python:python-src-import-error.ew-test_0.26.0a2@ --test_tag_filters=-none --cache_test_results=no

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 24, 2025

CodSpeed Performance Report

Merging #5407 will not alter performance

Comparing dominik/src-import-better-error (206f59a) with main (3527891)

Summary

✅ 33 untouched
⏩ 9 skipped1

Footnotes

  1. 9 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@dom96 dom96 force-pushed the dominik/src-import-better-error branch from 444565f to 206f59a Compare October 24, 2025 15:56
@dom96 dom96 marked this pull request as ready for review October 24, 2025 15:59
@dom96 dom96 requested review from a team as code owners October 24, 2025 15:59
exc.add_note(
"If your main module is inside the 'src' directory then your import " +
"statement shouldn't include a 'src.' prefix")
raise exc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid memory leaks you should do:

try:
   raise exc
finally:
   del exc

Or ideally move the whole thing into a Python try/catch.
It's important to remember that exceptions hold references to frame objects so keeping them alive also keeps all the local variables from all those scopes alive.

@hoodmane
Copy link
Contributor

What about if the import happens in the fetch handler?

pyodide.runPython(`
import sys
exc = sys.last_value
if exc.name == "src":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will raise an attribute error if exc doesn't have a name attribute. You need to check if it's a ModuleNotFoundError.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the test suite is currently passing let's make sure there's a test where some other error is raised at top level and make sure it doesn't get messed up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants